-
-
Notifications
You must be signed in to change notification settings - Fork 845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop urllib3 in favor of public gist #1182
Conversation
dd08a60
to
44c52a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. We can remove the --omit=httpx/_transports/urllib3.py
from scripts/coverage
now.
Fantastic! Let's hold off on pulling in API changes until we're ready to roll a major release. We could consider maintaining a release branch? |
The only other tiny tweak I can think of here in the gist would be changing from this: except (urllib3.exceptions.SSLError, socket.error) as exc:
tb = exc.__traceback__
raise httpcore.NetworkError(exc).with_traceback(tb) from None To this: except (urllib3.exceptions.SSLError, socket.error) as exc:
raise httpcore.NetworkError(exc) from exc Which I think gets us the traceback behaviour we actually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, it's got a do-not-merge
label on, which makes sense, but otherwise yup this is ace.
So, approved.
Do we think we ought to be maintaining a version branch so that we can pull this in, and then maintain the version branch until we're ready to go?
In the gist, let's just use
Once that's tweaked let's get this merged! |
I'd suggest we also include a link in the "third party packages" docs. |
✅
✅ Also dropped the recommendation to use Edit: also updated the README and docs home page to not mention |
|
||
<!-- NOTE: this list is in alphabetical order. --> | ||
|
||
### urllib3-transport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too keen to put this under "Plugins", since it's not really an installable package, but then also not keen on the existing "Plugins" section — perhaps we ought to just list everything as h2-level headers. We can address this as a follow-up.
Fantastic. 🌟 |
Fixes #1143
This PR removes the
URLLib3Transport
from HTTPX, and moves it to a public gist that we're now pointing to in the "Custom transports" docs.Also adds a small additional section to the Requests compatibility guide, so that people know this option exists if they're migrating from Requests to HTTPX.